Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Synchronous Child Datastore Creation #22962

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

anthony-murphy
Copy link
Contributor

@anthony-murphy anthony-murphy commented Nov 1, 2024

This change introduces a new pattern for datastore creation that is synchronous, specifically it allows the synchronous creation of a datastore from another datastore when the datastore being created is a child available synchronously via the existing datastore's registry, and the child's factory support synchronous creation. In addition to being synchronous, this method of datastore creation is also strongly typed for the consumer.

@github-actions github-actions bot added area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc public api change Changes to a public API base: main PRs targeted against main branch labels Nov 1, 2024
Copy link
Collaborator

@msfluid-bot msfluid-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Coverage Summary

↑ packages.runtime.container-runtime.src:
Line Coverage Change: 0.03%    Branch Coverage Change: 0.04%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 92.90% 92.94% ↑ 0.04%
Line Coverage 94.50% 94.53% ↑ 0.03%

Baseline commit: 54f2a18
Baseline build: 304086
Happy Coding!!

Code coverage comparison check passed!!

@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Nov 1, 2024

@fluid-example/bundle-size-tests: +3.46 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 464.2 KB 465.06 KB +874 Bytes
azureClient.js 562.44 KB 563.26 KB +843 Bytes
connectionState.js 724 Bytes 724 Bytes No change
containerRuntime.js 261.86 KB 262.72 KB +886 Bytes
fluidFramework.js 424.73 KB 424.75 KB +14 Bytes
loader.js 134.17 KB 134.18 KB +14 Bytes
map.js 42.71 KB 42.71 KB +7 Bytes
matrix.js 148.54 KB 148.54 KB +7 Bytes
odspClient.js 528.29 KB 529.11 KB +838 Bytes
odspDriver.js 97.84 KB 97.86 KB +21 Bytes
odspPrefetchSnapshot.js 42.81 KB 42.82 KB +14 Bytes
sharedString.js 164.75 KB 164.76 KB +7 Bytes
sharedTree.js 415.19 KB 415.2 KB +7 Bytes
Total Size 3.36 MB 3.36 MB +3.46 KB

Baseline commit: 54f2a18

Generated by 🚫 dangerJS against ee11048

@github-actions github-actions bot added the area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct label Nov 1, 2024
@anthony-murphy anthony-murphy marked this pull request as ready for review November 1, 2024 20:50
@anthony-murphy anthony-murphy requested a review from a team as a code owner November 1, 2024 20:50
@github-actions github-actions bot removed the area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct label Nov 1, 2024
@@ -523,6 +530,43 @@ export abstract class FluidDataStoreContext
return factory;
}

createChildDataStoreSync<T extends IFluidDataStoreFactory>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning for:

  1. Putting this on FluidDataStoreContext instead of ContainerRuntime, given that ContainerRuntime currently has the async createDataStore?
  2. Requiring it to be a child?

Copy link
Contributor Author

@anthony-murphy anthony-murphy Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the context has access to its factory synchronously, as its factory must be loaded before it is realized. This is basically the same pattern we use for dds.

*
* @param childFactory - The factory of the datastore to be created.
*/
createChildDataStoreSync?<T extends IFluidDataStoreFactory>(
Copy link
Contributor

@ChumpChief ChumpChief Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: I meant to put this comment in dataStoreFactory.ts, oops :)

I don't think I understand what the motivating scenario is for this PR, so I'd love to learn more about that. This comment is purely taking this PR at face value, so if I'm missing some special context let me know.

At a very high level, I'd question whether a sync factory pattern is moving in a good direction for our overall instantiation flows.

One pain point of Aqueduct (and therefore almost all of our current data store examples) is that the async startup flows are deferred to the data store instance (i.e. async initializingFirstTime, async hasInitialized). This leaves the instance in a prolonged "half-initialized" state which is prone to errors that don't account for certain invariants being violated.

Fluid basically mandates async somewhere in the construction flow though, due to retrieving handles, getChannel, etc.), so we can't go fully sync all the way to full initialization for most scenarios.

An alternative approach (ditching Aqueduct) would be to embrace an async factory, and move all async initialization up to the factory such that by the time we call the instance constructor, we are ready to be fully sync (and the instance is fully initialized in the constructor).

I've explored this pattern in https://github.com/microsoft/FluidFramework/blob/main/examples/utils/migration-tools/src/migrationTool.ts if you'd like an example. Fully admit there's some personal preference here, but I think this aligns better with DI principles, simplifies the instance class, and strengthens the instance class's invariants.

Copy link
Contributor Author

@anthony-murphy anthony-murphy Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scriptor has a case where they want to create a child datastore in response to user input, so there is no opportunity to do async actions.

i'd not confuse this with all instantiation, which include load, load must always be async. Create on the other hand can be synchronous, and that is what is enabled here. This matches how dds work.

We can't split the sync part and the async part here. the whole things need to be sync, as we need a net new datastore synchronously that ready to be used, and have its handle stores in the same js turn, to not break on user input.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you - that makes more sense.

* @legacy
* @alpha
*/
export type NamedFluidDataStoreRegistryEntry2 = [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a new type is the most compatible thing according to our type tests, as it typescript has trouble with inference over typed arrays. not sure what to call this, everything else feels quite verbose, so kind of happy with just adding 2

@@ -3,7 +3,10 @@
* Licensed under the MIT License.
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i basically rewrote this test to split the parent and child datastore, and handle more cases, so i'd recommend split view, to really just review the latest.

Copy link
Contributor

github-actions bot commented Nov 5, 2024

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluidframework-docs@0.25.0 ci:linkcheck /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test ci:start 1313 linkcheck:full

1: starting server using command "npm run ci:start"
and when url "[ 'http://127.0.0.1:1313' ]" is responding with HTTP status code 200
running tests using command "npm run linkcheck:full"


> fluidframework-docs@0.25.0 ci:start
> http-server ./public --port 1313 --silent


> fluidframework-docs@0.25.0 linkcheck:full
> npm run linkcheck:fast -- --external


> fluidframework-docs@0.25.0 linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

Stats:
  441702 links
    3405 destination URLs
       2 URLs ignored
       0 warnings
       0 errors


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants